-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HSEARCH-3667 Envers compatibility #2067
Conversation
I've create this pull request to fix an issue when using Hibernate Search with Envers. However I've got two remarks to do about my PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this patch. The implementation looks good, I only have a minor comment.
I'm not overly concerned about the hardcoded class name as long as we implement a test for it, but as I mentioned below I'm not sure this part of the PR is necessary, so it might be hard to test :)
Regarding tests, we don't currently have a dependency to Envers in our tests and I think it would be wise to keep it that way, so that we're sure Search works correctly without any other dependency.
But obviously we need to test compatibility with Envers, so I'd be inclined to create a separate module for that:
- Create a new folder:
integrationtest/mapper/orm-envers
- Create a POM in this folder. You can copy/paste the one from
integrationtest/mapper/orm
and adapt it. - Reference the module from
integrationtest/pom.xml
: add a<module>mapper/orm-envers</module>
line in the<modules>
section. - Create the directory structure of your new module. The root package should probably be
org.hibernate.search.integrationtest.mapper.orm.envers
- Create a test. You can take
org.hibernate.search.integrationtest.mapper.orm.model.GenericPropertyIT
as an example, it's not too complicated and should be close to what you'll need for the Envers test. Just remove the entity classes other thanIndexedEntity
, makeIndexedEntity
auditable with a single, indexed text property, and adapt the schema expectations insetup
and document expectations inindex
. - Run the test:
mvn clean install -pl 'integrationtest/mapper/orm-envers' -am
There are two things to keep in mind when dealing with ORM integration tests:
-
Hibernate ORM bootstrap is handled by a helper (
ormSetupHelper
in your test), which offers simple configuration APIs adapted to our use cases, so we don't have to copy-paste tons of code in every single test. -
The tests are backend-independent: they do not use Lucene or Elasticsearch. Instead, they use a "stub" backend, which will simply check that the information sent by Hibernate Search to the backend is the one we expect. Thus, in your test, you will have to define an expected index schema, and an expected document when indexing. To that end, you will call a method on the
backendMock
, and define the schema/document using a lambda accepting a builder:- For the schema,
backendMock.expectSchema( IndexedEntity.INDEX, b -> b.field( "text", String.class ) )
defines the expectation that the schema of the index namedIndexedEntity.INDEX
will only contain a single field named "text", of typeString
, with default options. An assertion error will be thrown if this doesn't happen, or if a different schema is defined. - For the document,
backendMock.expectWorks( IndexedEntity.INDEX ).add( "1", b -> b.field( "text", "some text" )
defines the expectation that a document will be added to the index with the identifier "1" and with a single field named "text" containing the value "some text". An assertion error will be thrown if this doesn't happen, or if a different document is indexed.
- For the schema,
If you need any further help, don't hestitate to ask.
.../main/java/org/hibernate/search/mapper/orm/model/impl/HibernateOrmBootstrapIntrospector.java
Outdated
Show resolved
Hide resolved
@dcdh Hello! Do you think you'll be able to add a test to this PR, so we can merge it? In case you want to do it, I provided some guidance in my earlier comment regarding how to add the test. If you don't have the time, please let us know so we can plan accordingly. Thanks. |
Hello Yoann,
I'm sorry for my delay to response. I was quite busy.
I will give you tonight some remarks about it.
Regards,
Damien Clément d'Huart
Le jeu. 5 sept. 2019 à 09:15, Yoann Rodière <notifications@github.com> a
écrit :
… @dcdh <https://github.com/dcdh> Hello!
Do you think you'll be able to add a test to this PR, so we can merge it?
In case you want to do it, I provided some guidance in my earlier comment
regarding how to add the test.
If you don't have the time, please let us know so we can plan accordingly.
Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2067?email_source=notifications&email_token=ABHS735IGIQJCNWXVB6M6DTQICWY7A5CNFSM4IPXGLP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD56DDNQ#issuecomment-528232886>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABHS737NXGYKJWZG3VLJQ5DQICWY7ANCNFSM4IPXGLPQ>
.
|
Hello Yoann, Sorry for the late reply. I'm working on my free time to integrate hibernate ORM + Envers + Search under Quarkus. Regarding Hibernate it seems that Hibernate Search is decoupled from Hibernate Core. The link is made in Quarkus via the Hibernate Search 6 is a full rewrite, some code remains in legacy dealing with Envers so maybe that in a futur a bootstrap between Search and Envers will be provided avoiding vendor to create a custom one ? I may be wrong. So I admit I am stuck on testing this PR... |
Ok, thanks for the answer. We'll give it a try. |
Avoid to process no JPA entity tables like hibernate Envers audited tables.
22778fa
to
3ee4cb2
Compare
3ee4cb2
to
d9699db
Compare
Alright, I implemented a few (simple) tests, ported from Search 5. Goods news is: your patch does solve the problem, and Envers seems to be working now. Thank you very much for bringing this to our attention and taking the time to fix it! I just launched a full build of this PR, testing in all supported environments (multiple databases, in particular). Let's see how it turns out, and if it succeeds I will merge. Regarding Quarkus, one thing to keep in mind is that Quarkus takes Hibernate Search and performs extra steps to integrate it and make it compatible with all of its improvements (native images, compile-time boot, ...). The integration tests in the Hibernate Search project only run on a "traditional" hotspot JVM without any of the Quarkus bits. So in the context of this PR, Quarkus is not relevant. That being said, there will be a need to test the Envers integration in Quarkus. I think you should be able to discuss that in quarkusio/quarkus#3690 . @gsmet is the person to talk to if anything doesn't work. |
Merged! This fix will be part of the next release. Thanks again! |
https://hibernate.atlassian.net/browse/HSEARCH-3667